Skip to content

Implement multi-parent packed scan for CSR rel tables - #867

Open
adsharma wants to merge 4 commits into
mainfrom
fix_asan
Open

Implement multi-parent packed scan for CSR rel tables#867
adsharma wants to merge 4 commits into
mainfrom
fix_asan

Conversation

@adsharma

Copy link
Copy Markdown
Contributor

Summary

Implements the deferred design from docs/multi_parent_lifetime.md: a multi-parent packed scan for the CSR rel scan path, plus the lifetime/representation documentation work it calls for.

Representation (PackedChildSlices)

  • parentPositions (owned copy of only non-empty parents) → shared_ptr<SelectionVector> aliasing the bound (parent) chunk state's selection vector + prefix-sum offsets over all served parents (offsets.size() == parentSelSize + 1).
  • Zero-length ranges (offsets[i] == offsets[i+1]) are allowed and must be skipped by consumers — the "drop parents without matches" invariant moves from scan time to the consumer, exactly as the doc describes.
  • The shared_ptr keeps the SelectionVector object alive while still aliasing mutable contents, preserving the synchronous-consumption lifetime rule.

Storage (CSRNodeGroup)

  • New CSRNodeGroupScanState::tryScanCachedTuplesPacked(): serves children of as many consecutive parents as fit into one output batch from the CSR row cache. Builds the combined (filtered) output selection vector, the per-parent prefix sum (RelTableScanState::packedChildOffsets), and the served-parent selection on the bound chunk state (switched to unflat).
  • Handles zero-length CSR lists, capacity-limited partial lists, and cache-window boundaries; a parent whose list spans cache loads is resumed seamlessly across batches.
  • All other scan paths (without-cache, in-memory, local storage, arrow/ice-disk/foreign) keep the one-parent-per-batch contract.

Operator & gating

  • ScanRelTable::updatePackedChildSlices() attaches the descriptor for both batch shapes (derived from the bound vector's selection size); the speculative append/reserve machinery is removed.
  • Multi-parent batches change the factorization contract (bound chunk unflat holding the served parents), so they are opt-in per consumer: the plan mapper enables it only for PackedFilteredCount consuming the scan output directly (dynamic_cast on the mapped child). Plans that fetch an nbr node property route through a hash join + FactorizedTable materialization — the exact materialization hazard the doc warns about — and correctly stay single-parent (verified empirically).

Consumer

  • PackedFilteredCount attributes counts per parent via the packed slices when present (a batch may span many group keys); the legacy single-tuple cross-product path is preserved when no descriptor is attached (e.g. when the sink sits above the property-fetch hash join).

Tests

  • Unit tests reworked for the new representation + a dedicated test documenting the aliasing semantics.
  • New storage-level test drives RelTable::scan directly with packing enabled over a 3000-node / 6750-edge graph (degree 3, with zero-child parents): asserts multi-parent batches actually occur, the prefix-sum invariants hold, and every edge is served exactly once with correct parent attribution.
  • SQL-level equivalence: the PackedFilteredCount pattern over an nbr property (join pipeline, single-parent batches) produces identical results with the packed extend on and off.

Docs

  • docs/multi_parent_lifetime.md updated with an implementation-status note, including the materialization-boundary constraint that keeps the property-join plan shape single-parent.

Testing

  • planner_tests (release + ASAN, DASSERTs exercised), rel_tests, and ~50 e2e test groups (match, filter, agg, generic_hash_join, dml_rel, cyclic, path, subquery, transaction, unwind, rel_group, acc, projection, …) all pass.
  • clang-format-18 clean.

The sanitizer test suite needs the dataset submodule for the demo db;
mirror the pattern used in ci-workflow.yml.
Implement docs/multi_parent_lifetime.md's "for now" recommendation:

- Keep the owned std::vector<sel_t> parentPositions copy in
  PackedChildSlices; explicitly document why a pointer/alias into the
  parent's SelectionVector is unsafe (contents rewritten in place by
  setToFiltered/setToUnfiltered).
- Document the synchronous-consumption lifetime rule: the descriptor is
  valid only for the current output batch and must not be persisted
  across a materialization boundary.
- Record the deferred multi-parent convention (shared_ptr sel vector +
  prefix-sum offsets over all parents, zeros allowed) for when the
  multi-parent packed scan is implemented.
- Add a unit test codifying the owned-copy invariant against in-place
  sel vector mutation.
Implement the deferred design from docs/multi_parent_lifetime.md:

Representation (PackedChildSlices):
- Replace the owned parentPositions copy with a shared_ptr alias to the
  bound (parent) chunk state's selection vector plus a prefix-sum
  offsets vector over ALL served parents (offsets.size() ==
  parentSelSize + 1). Zero-length ranges (parents without children in
  the batch) are allowed; consumers skip them. The shared_ptr keeps the
  SelectionVector object alive while still aliasing mutable contents,
  preserving the synchronous-consumption lifetime rule.

Storage (CSRNodeGroup):
- Add tryScanCachedTuplesPacked: serves children of as many consecutive
  parents as fit into one output batch from the row cache, building the
  combined (filtered) output selection vector, the per-parent prefix
  sum in RelTableScanState::packedChildOffsets, and the served-parent
  selection on the bound chunk state (switched to unflat). Handles
  zero-length lists, capacity-limited partial lists, and cache-window
  boundaries; parents whose lists span batches are resumed seamlessly.
- Single-parent serves (without-cache, in-memory, local storage) keep
  the existing one-parent-per-batch contract.

Operator (ScanRelTable):
- Attach the descriptor for both single- and multi-parent batches; the
  batch shape is derived from the bound vector's selection size.
- Drop the speculative append/reserve machinery (superseded by direct
  offsets construction).

Consumer gating:
- Multi-parent batches change the factorization contract (bound chunk
  unflat with the served parents), so they are opt-in per consumer:
  the plan mapper enables it only for PackedFilteredCount consuming
  the scan output DIRECTLY (no FactorizedTable/hash-join between, the
  exact materialization hazard the doc warns about). Plans that fetch
  nbr node properties through the property-join pipeline keep
  single-parent batches.

PackedFilteredCount:
- Attribute counts per parent via the packed slices when present (a
  batch may span many group keys); keep the legacy single-tuple path
  when no descriptor is attached.

Tests (test/planner/cardinality_test.cpp):
- Rework the DataChunkState unit tests for the new representation and
  document the aliasing semantics in a dedicated test.
- Add a storage-level test driving RelTable::scan directly with
  packing enabled over a 3000-node/6750-edge graph (degree 3, with
  zero-child parents): asserts multi-parent batches actually occur,
  the offsets prefix-sum invariants hold, and every edge is served
  exactly once with correct parent attribution.
- Add SQL-level equivalence: the PackedFilteredCount pattern over an
  nbr property (join pipeline, single-parent batches) produces
  identical results with the packed extend on and off.

Docs: mark the deferred design as implemented in
docs/multi_parent_lifetime.md, noting the materialization boundary
constraint that keeps the property-join plan shape single-parent.
Debug builds assert in FactorizedTable::copyVectorToUnflatColumn /
appendVectorToUnflatTupleBlocks when a flat vector is appended to an
unflat column. The implementation already handles this correctly: a
flat vector (single selected value) is stored as a one-element unflat
overflow value, which is exactly the right representation.

This shape is reached by the packed filtered count pipeline: the packed
extend keeps its bound node group unflat at plan time
(getGroupsPosToFlatten is empty, so no Flatten operators are inserted),
while the CSR scan presents the bound chunk flat with selSize 1 per
output batch. When the acc-hash-join SIP rewrite places an Accumulate
(FactorizedTable writer) directly above the packed extend, the append
hits the over-strict assert on DASSERT-enabled (Debug) builds. Release
builds run the same path and produce correct results.

Relax the asserts to require only that a flat vector carry a single
selected value, matching the implemented semantics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant